[SYCL] Remove secondary submission queue from handler #17967
Conversation
uditagarwal97
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@jbrodman FYI |
sycl/source/detail/handler_impl.hpp
Outdated
| bool EventNeeded) | ||
| handler_impl( | ||
| std::shared_ptr<queue_impl> SubmissionPrimaryQueue, | ||
| [[maybe_unused]] std::shared_ptr<queue_impl> SubmissionSecondaryQueue, |
There was a problem hiding this comment.
Is there a reason to keep the secondary queue argument in handler_impl? Can we remove it here, and just keep it in the handler for ABI compatibility?
There was a problem hiding this comment.
That's a good idea. Applied in 14dc5ac
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
changed the title
sycl/source/handler.cpp
Outdated
| return false; | ||
| } | ||
| return true; | ||
| assert(impl->MSubmissionPrimaryQueue && |
There was a problem hiding this comment.
@slawekptak Since the last review, I've also removed null check and converted to assert instead. It's unlikely to have a null primary queue in handler.
There was a problem hiding this comment.
Thanks for the info. Let me share one more thought/question. It seems like handler_impl can be constructed using a graph instead of queues. In this case, this function (and other 2) used to return true, now they would crash/assert. The question is, are these functions used at all with the graph version of the handler? Please note that the use_kernel_bundle function would check if the graph is there, and in this case get the context from the graph instead of the queue.
There was a problem hiding this comment.
It seems like handler_impl can be constructed using a graph instead of queues.
I didn't know that handler_impl ca be constructed using a graph. You're right, we should not assert in that case. My whole assumption was that we can't have a handler without a queue.
There was a problem hiding this comment.
@slawekptak I've re-added the null check in 0757546
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Follow up of #17967 Secondary submission queue is optional in SYCL Spec and it has been decided to remove it.
handler's secondary queue is not used for any meaningful work.